Skip to content

Add listing field with CardPill and change action to create PR modal #4287

Merged
tintinthong merged 2 commits into
mainfrom
CS-10556/create-pr-modal-should-display-a-field-to-linked-to-the-lisitng
Apr 1, 2026
Merged

Add listing field with CardPill and change action to create PR modal #4287
tintinthong merged 2 commits into
mainfrom
CS-10556/create-pr-modal-should-display-a-field-to-linked-to-the-lisitng

Conversation

@lucaslyl
Copy link
Copy Markdown
Contributor

linear: https://linear.app/cardstack/issue/CS-10556/create-pr-modal-should-display-a-field-to-linked-to-the-lisitng

Replace static listing name and realm fields with a CardPill component that links to the listing, and add a "Change" button to select a different listing via the catalog chooser.

Demo

Screen.Recording.2026-03-31.at.10.25.50.PM.mov

lucaslyl and others added 2 commits March 31, 2026 22:25
…modal

Replace static listing name and realm fields with a CardPill component
that links to the listing, and add a "Change" button to select a
different listing via the catalog chooser.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@lucaslyl lucaslyl self-assigned this Mar 31, 2026
@lucaslyl
Copy link
Copy Markdown
Contributor Author

@codex review

@github-actions
Copy link
Copy Markdown
Contributor

Preview deployments

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2ce376cf64

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +143 to +147
<CardPill
@cardId={{this.listingId}}
@urlForRealmLookup={{this.listingId}}
@displayTitle={{this.listingTitle}}
class='listing-pill'
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Wire listing pill click to open selected listing

The new listing field renders CardPill without an @onClick handler, so it stays non-interactive and does not navigate anywhere. In CardPill, click behavior is only enabled when @onClick is provided, so this change does not actually provide a linked listing field; users cannot open/verify the selected listing before submitting the PR.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want users to click to view the listing in the modal. The purpose of this field is to allow users to select a listing for submission only. So the current non-interactive behavior is correct

@lucaslyl lucaslyl requested a review from a team March 31, 2026 14:56
@github-actions
Copy link
Copy Markdown
Contributor

Realm Server Test Results

  1 files  ±0    1 suites  ±0   11m 44s ⏱️ -23s
802 tests ±0  802 ✅ ±0  0 💤 ±0  0 ❌ ±0 
873 runs  ±0  873 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 2ce376c. ± Comparison against base commit 20a185a.

@github-actions
Copy link
Copy Markdown
Contributor

Host Test Results

    1 files  ±0      1 suites  ±0   2h 13m 13s ⏱️ - 1m 11s
2 065 tests +3  2 050 ✅ +3  15 💤 ±0  0 ❌ ±0 
2 080 runs  +3  2 065 ✅ +3  15 💤 ±0  0 ❌ ±0 

Results for commit 2ce376c. ± Comparison against base commit 20a185a.

This pull request removes 2 and adds 5 tests. Note that renamed tests count towards both.
Chrome ‑ Integration | components | create-pr-modal: shows listing name in modal
Chrome ‑ Integration | components | create-pr-modal: shows realm info in modal
Chrome ‑ Integration | components | create-pr-modal: cancel button dismisses the modal
Chrome ‑ Integration | components | create-pr-modal: does not show a separate realm field in modal
Chrome ‑ Integration | components | create-pr-modal: does not show change action when catalog chooser is unavailable
Chrome ‑ Integration | components | create-pr-modal: shows the listing pill in modal
Chrome ‑ Integration | components | create-pr-modal: submit shows success state

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the operator-mode “Create PR” modal to display the selected listing as a CardPill (instead of separate listing/realm text fields) and introduces a “Change” action to re-select a listing via the catalog chooser.

Changes:

  • Replace static listing/realm display with a CardPill in the Create PR modal UI.
  • Add “Change” listing action powered by chooseCard, and update submission to use the selected listing.
  • Expand integration tests to cover updated modal behaviors (pill, absence of realm field, cancel, submit success).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
packages/host/app/components/operator-mode/create-pr-modal.gts Renders listing as CardPill, adds “Change” listing chooser flow, adjusts submission payload and modal copy/styles.
packages/host/tests/integration/components/create-pr-modal-test.gts Updates/extends modal integration tests for the new UI and behaviors.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +142 to +148
{{#if this.listingId}}
<CardPill
@cardId={{this.listingId}}
@urlForRealmLookup={{this.listingId}}
@displayTitle={{this.listingTitle}}
class='listing-pill'
/>
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The listing “pill” is rendered as a non-interactive CardPill (no @OnClick), so it doesn’t actually link/open the listing as described in the PR/issue. Consider wiring an @onClick handler (e.g. using operatorModeStateService.openCardInInteractMode(this.listingId, 'isolated')) or otherwise making the pill an anchor/button so users can navigate to the listing from the modal.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@lucaslyl lucaslyl Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The CardPill is used as a label/indicator, same pattern as attachments.gts which also uses CardPill without @OnClick
  2. The modal already has a "Change" button for interaction — making the pill itself clickable would be redundant
  3. The PR modal's purpose is to create a PR, not to navigate to the listing — opening the listing would take the user away from the modal flow

Comment on lines +70 to +71
let realm =
this.realm.realmOfURL(new URL(currentListingId))?.href ?? payload.realm;
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When submitting after a listing has been changed, realmOfURL(new URL(currentListingId)) can return undefined if that realm isn’t currently in realm.realms, and the code silently falls back to payload.realm. That can submit a PR request against the wrong realm. Safer options: (a) throw if the realm can’t be derived for the selected listing, or (b) derive the realm deterministically from currentListingId (instead of falling back to the original payload realm).

Suggested change
let realm =
this.realm.realmOfURL(new URL(currentListingId))?.href ?? payload.realm;
let realmURL = this.realm.realmOfURL(new URL(currentListingId));
if (!realmURL) {
throw new Error('Cannot determine realm for selected listing');
}
let realm = realmURL.href;

Copilot uses AI. Check for mistakes.
assert.dom('[data-test-create-pr-realm]').exists();
assert.dom('[data-test-create-pr-change-listing-button]').doesNotExist();
});

Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new “Change” listing workflow isn’t covered by tests (only the absence case is asserted). Add a test that makes the chooser available (and stubs chooseCard to return a different listingId), clicks the Change button, and asserts that the pill updates and the submit action uses the newly selected listingId.

Suggested change
test('can change listing via catalog chooser and submit with new listingId', async function (assert) {
await renderComponent(
class TestDriver extends GlimmerComponent {
<template><OperatorMode @onClose={{noop}} /></template>
},
);
ctx.operatorModeStateService.showCreatePRModal({
realm: testRealmURL,
listingId: `${testRealmURL}Listing/1`,
listingName: 'My Listing',
});
// Make the catalog chooser available and stub chooseCard to return a different listing
let newListingId = `${testRealmURL}Listing/2`;
let newListingName = 'My Other Listing';
// Depending on how setupOperatorModeTests exposes the chooser, adapt this stub:
// here we assume a chooser object on the test context.
// @ts-ignore - test-time mutation
ctx.catalogChooser = ctx.catalogChooser || {};
// @ts-ignore - test-time mutation
ctx.catalogChooser.isAvailable = true;
// @ts-ignore - test-time mutation
ctx.catalogChooser.chooseCard = async () => {
return {
listingId: newListingId,
listingName: newListingName,
};
};
let submittedListingId: string | null = null;
// Stub the submit handler to capture the listingId used when submitting
// @ts-ignore - test-time mutation
ctx.operatorModeStateService.submitCreatePR = (options: { listingId: string }) => {
submittedListingId = options.listingId;
};
await waitFor('[data-test-create-pr-modal]');
// Initially shows the original listing
assert.dom('[data-test-create-pr-listing-name]').includesText('My Listing');
// Click the Change button to open the chooser and select the new listing
await click('[data-test-create-pr-change-listing-button]');
// After choosing, the pill should update to the new listing name
assert.dom('[data-test-create-pr-listing-name]').includesText(newListingName);
// Submit the PR and assert the new listingId is used
await click('[data-test-create-pr-submit-button]');
assert.strictEqual(submittedListingId, newListingId, 'submit uses newly selected listingId');
});

Copilot uses AI. Check for mistakes.
@tintinthong tintinthong merged commit 109dc7a into main Apr 1, 2026
60 of 61 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants